-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C++: Cut down on conversions between dbtypes and Element #110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The presence of abstract predicates on this class made it hard to specialize it, and this is one of the reasons why the raw db-type `@cfgnode` is often used in preference to `ControlFlowNode`.
Many classes have been declared with `extends @cfgnode` because they should be implemented internally as a control-flow node but should not expose the member predicates of `ControlFlowNode` to their users. After the transition in a1e4404 it became mandatory to convert explicitly between the `Element`-derived `ControlFlowNode` and the raw dbtype `@cfgnode`, and that commit inserted numerous such conversions as a result of having all those classes that did not derive from `Element` in the standard library. It was also confusing and error-prone that the libraries implementing `ControlFlowNode` referred to `ControlFlowNode`. This seemingly cyclic reference worked out because the libraries did not call the predicates on `ControlFlowNode` whose implementation they were part of. Both these problems are now solved by adding a new class `ControlFlowNodeBase extends Element` that should be used in preference to `@cfgnode` everywhere. This class is for exactly those use cases where `@cfgnode` should be seen as an `Element` without having too many member predicates on it. The classes that move from extending `@cfgnode` to extending `ControlFlowNodeBase` are: `BasicBlock`, `AdditionalControlFlowEdge`, `DefOrUse`, `SsaDefinition`, `SubBasicBlock` and `RangeSsaDefinition`. These previously had to define their own `toString` rootdef, which typically had some dummy string as result (like `"BasicBlock"`), but now their `toString` is part of the `Element` rootdef and should not be overridden otherwise `Element.toString` will sometimes have multiple results. Removing these dummy `toString` predicates had some effects on the tests that are included in this commit. The `getLocation` family of predicates is affected like `toString`, but the situation is slightly different. Some of these classes had genuinely useful alternative definitions of locations. Fortunately, they all used `hasLocationInfo`, which is preferred over `getLocation` by the QL engine. Because `Element` does not define `getLocationInfo`, each class can create its own rootdef of this predicate like before.
This should have no effect in itself but changes the test output to correspond with the change coming next.
This avoids using a raw db type. It is possible for `SuppressionComment` and `SuppressionScope` to have different locations because `SuppressionScope` defines `hasLocationInfo` as a new rootdef whereas `SuppressionComment` only responds to `getLocation` that it inherited. In interpretation of query results, a `hasLocationInfo` predicate is preferred over `getLocation` if it exists.
This changes the test output because `VariableDeclarationGroup.toString` changes to be the one inherited from VariableDeclarationEntry. This should not affect the output as shown by any front end because the string to be displayed to the user for a `$@` interpolation comes from the following column instead.
These were made redundant when a1e4404 changed their parent class to extend `Element`.
By extending this class, a class can define its own `getLocation` predicate without participating in the dispatch hierarchy of `getLocation` as defined on `Element`. Classes wanting to override their location previously had to define `getURL` or `hasLocationInfo` instead and rely on these predicates not being defined on future versions of `Element`.
ian-semmle
reviewed
Aug 29, 2018
*/ | ||
class Element extends ElementBase { | ||
Element() { | ||
this = resolveElement(_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now redundant
ian-semmle
previously approved these changes
Aug 29, 2018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM
ian-semmle
approved these changes
Aug 29, 2018
aibaars
pushed a commit
that referenced
this pull request
Oct 14, 2021
Add AST classes for classes and modules
smowton
added a commit
to smowton/codeql
that referenced
this pull request
Dec 6, 2021
Fix Kotlin style snags
dbartol
pushed a commit
that referenced
this pull request
Dec 18, 2024
Expect external workflows and actions in .github/workflow/external and .github/actions/external
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a follow-up to #7, which littered the code with calls the
resolveElement
family of predicates. These were mechanically added to support the hypotheticalnewtype
-ification ofElement
while mostly preserving the code unmodified. This PR does some much overdue refactoring that removes many of these conversions by avoiding the use of raw dbtypes where they are not needed.I've tried to make each commit self-contained, so I recommend reviewing them one by one. Another good way to look at this PR is relative to the parent of a1e44041e, which shows what #7 looks like with this PR directly after it. Hopefully only strictly necessary conversions are present there, or at least the undisciplined ones come directly from undisciplined direct use of db relations.
I have not been able to test this with an actual
newtype
-ification ofElement
. Maybe you can do that, @ian-semmle, if you think it's important. In any case, strictly correct use of theresolveElement
predicates is likely to rot over time as we don't have any type checking of it yet.These QL changes caused no changes to
semmlecode-cpp-tests
in the internal repo. I have not looked into QL code in the internal repo, and I'm not sure it's important to do so now.